Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cythonize GraphicMatroid #37839

Merged
merged 12 commits into from
May 12, 2024
Merged

Cythonize GraphicMatroid #37839

merged 12 commits into from
May 12, 2024

Conversation

gmou3
Copy link
Contributor

@gmou3 gmou3 commented Apr 20, 2024

A metamorphosis of graphic_matroid.py into graphic_matroid.pyx and graphic_matroid.pxd.
Things should be somewhat faster; test, e.g., the GraphicMatroid's _circuit function. An indirect and diluted test that shows improvement can be seen by running %time matroids.CompleteGraphic(8).circuits() (~half the time).

For the reviewer: The changes of this PR are only inside the matroids module. The rest come from #37835.

⌛ Dependencies

#37835: Deeper improvements of DisjointSet.

@gmou3 gmou3 mentioned this pull request Apr 20, 2024
Merged
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 20, 2024 •
edited
Loading

Documentation preview for this PR (built with commit 9fda0f7; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@gmou3
Merge branch 'disjointset_optimize' into cythonize_graphicmatroid
0625153
@gmou3
Merge branch 'disjointset_optimize' into cythonize_graphicmatroid
07deb59
@gmou3
Various improvements
19a4efc 
Remove `_find` & `_union`.
Remove `DisjointSet_of_integers` `_d` attribute from `DisjointSet_of_hashables` (and adjust code).
Add notes recommending `OP_find` and `OP_union` if no input checking needed.
Touch-up docstrings.
@gmou3
Merge branch 'disjointset_optimize' into cythonize_graphicmatroid
2313a45
@gmou3
Improve GraphicMatroid's docstrings
a51c093
@gmou3 gmou3 requested a review from tscrim April 29, 2024 09:59
vbraun pushed a commit to vbraun/sage that referenced this pull request May 2, 2024
sagemathgh-37835: Optimize DisjointSet
ac9c1b0 
    
The methods `find` and `union` of the class `DisjointSet_of_hashables`
pass through an unnecessary layer of input checks that slow down the
code. I avoided these checks, cythonized more of the code, and edited
the docstrings.

I can attest to the fact that these changes improve speed, and in a
follow-up PR (sagemath#37839) I mention a test that confirms this.
    
URL: sagemath#37835
Reported by: gmou3
Reviewer(s): gmou3, Martin Rubey, Travis Scrimshaw, Vincent Delecroix
tscrim
tscrim reviewed May 5, 2024
View reviewed changes
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be good to look at the C code generated in performance critical sections to see if there are additional places where type casting and other such declarations could simplify the C code (and hence, increase performance). Although I am basically okay to include this in the current state and do other general improvements in later PRs. It is up to you.

src/sage/matroids/graphic_matroid.pyx Show resolved Hide resolved
src/sage/matroids/graphic_matroid.pyx Outdated Show resolved Hide resolved
@gmou3 @tscrim
Update src/sage/matroids/graphic_matroid.pyx
9fda0f7 
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
@gmou3
Copy link
Contributor Author

gmou3 commented May 5, 2024

It might be good to look at the C code generated in performance critical sections to see if there are additional places where type casting and other such declarations could simplify the C code (and hence, increase performance). Although I am basically okay to include this in the current state and do other general improvements in later PRs. It is up to you.

I hadn't thought of doing that and I'll keep the idea in mind for future PRs.

tscrim
tscrim reviewed May 5, 2024
View reviewed changes
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, then let's get this in. It seems like the tester failures are unrelated, but it would be good to have them be run... I don't know how to kick it to run it again.

@gmou3
Copy link
Contributor Author

gmou3 commented May 5, 2024

Please don't kick it as it may further break things.
There are many errors indeed; alas, what a state of disrepair!

@mkoeppe
Copy link
Contributor

mkoeppe commented May 8, 2024

I've repaired the CI, which complains:

sage -t --random-seed=109326302233194564031839857779385647036 src/sage/matroids/graphic_matroid.py
**********************************************************************
File "src/sage/matroids/graphic_matroid.py", line 490, in sage.matroids.graphic_matroid.GraphicMatroid._minor
Failed example:
    M._minor(deletions=frozenset([0,1,2]))
Exception raised:
    Traceback (most recent call last):
      File "/sage/src/sage/doctest/forker.py", line 714, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/sage/src/sage/doctest/forker.py", line 1146, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.matroids.graphic_matroid.GraphicMatroid._minor[1]>", line 1, in <module>
        M._minor(deletions=frozenset([Integer(0),Integer(1),Integer(2)]))
      File "sage/matroids/graphic_matroid.pyx", line 462, in sage.matroids.graphic_matroid.GraphicMatroid._minor
        cpdef _minor(self, contractions, deletions):
    TypeError: _minor() takes exactly 2 positional arguments (0 given)
**********************************************************************
File "src/sage/matroids/graphic_matroid.py", line 492, in sage.matroids.graphic_matroid.GraphicMatroid._minor
Failed example:
    M._minor(contractions=frozenset([0,1,2]))
Exception raised:
    Traceback (most recent call last):
      File "/sage/src/sage/doctest/forker.py", line 714, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/sage/src/sage/doctest/forker.py", line 11[46](https://github.com/sagemath/sage/actions/runs/8959227503/job/24712775461?pr=37839#step:12:47), in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.matroids.graphic_matroid.GraphicMatroid._minor[2]>", line 1, in <module>
        M._minor(contractions=frozenset([Integer(0),Integer(1),Integer(2)]))
      File "sage/matroids/graphic_matroid.pyx", line 462, in sage.matroids.graphic_matroid.GraphicMatroid._minor
        cpdef _minor(self, contractions, deletions):
    TypeError: _minor() takes exactly 2 positional arguments (1 given)
**********************************************************************

@tscrim
Copy link
Collaborator

tscrim commented May 9, 2024

Something is wrong with the CI. There is no more graphic_matroid.py file as it is changed to a .pyx file.

@mkoeppe
Copy link
Contributor

mkoeppe commented May 10, 2024

Indeed, sorry for the noise.

vbraun pushed a commit to vbraun/sage that referenced this pull request May 11, 2024
sagemathgh-37839: Cythonize GraphicMatroid
e5d4e1d 
    
A metamorphosis of `graphic_matroid.py` into `graphic_matroid.pyx` and
`graphic_matroid.pxd`.
Things should be somewhat faster; test, e.g., the `GraphicMatroid`'s
`_circuit` function. An indirect and diluted test that shows improvement
can be seen by running `%time matroids.CompleteGraphic(8).circuits()`
(~half the time).

`For the reviewer:` The changes of this PR are only inside the
`matroids` module. The rest come from sagemath#37835.

### ⌛ Dependencies

sagemath#37835: Deeper improvements of `DisjointSet`.
    
URL: sagemath#37839
Reported by: gmou3
Reviewer(s): gmou3, Kwankyu Lee, Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this pull request May 12, 2024
sagemathgh-37839: Cythonize GraphicMatroid
632ff36 
    
A metamorphosis of `graphic_matroid.py` into `graphic_matroid.pyx` and
`graphic_matroid.pxd`.
Things should be somewhat faster; test, e.g., the `GraphicMatroid`'s
`_circuit` function. An indirect and diluted test that shows improvement
can be seen by running `%time matroids.CompleteGraphic(8).circuits()`
(~half the time).

`For the reviewer:` The changes of this PR are only inside the
`matroids` module. The rest come from sagemath#37835.

### ⌛ Dependencies

sagemath#37835: Deeper improvements of `DisjointSet`.
    
URL: sagemath#37839
Reported by: gmou3
Reviewer(s): gmou3, Kwankyu Lee, Travis Scrimshaw
@vbraun vbraun merged commit 0e8fd93 into sagemath:develop May 12, 2024
19 of 22 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone May 12, 2024
@gmou3 gmou3 deleted the cythonize_graphicmatroid branch May 25, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kwankyu kwankyu kwankyu left review comments

@tscrim tscrim tscrim approved these changes

Assignees
No one assigned
Labels
c: cython c: matroid theory t: performance
Projects
None yet
Milestone
sage-10.4
Development

Successfully merging this pull request may close these issues.
5 participants
@gmou3 @mkoeppe @tscrim @kwankyu @vbraun